Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

csharp: improve performance and reduce memory footprint when creating and verifying webhook signatures #1616

Merged
merged 12 commits into from
Jan 10, 2025

Conversation

esskar
Copy link
Contributor

@esskar esskar commented Jan 5, 2025

Using latest csharp and dotnet features to increase performance and reduce memory footprint when creating and verifying webhook signatures

Motivation

When using the webhook verification process we see lots of memory wasted and garbage collector kicking when our applications gets hit with a lot of webhook requests.

Solution

  • using ReadOnly instead of string in Webhook.Verify and Webhook.Sign
  • using stack alloctions instead of heap allocations when creating and verifing signatures as much as possible
  • using new Base64 API to convert to base64 operating on spans

Benchmarks

[MemoryDiagnoser]
public class Tests
{
    private readonly string _msgId = "msg_p5jXN8AQM9LWM0D4loKWxJek";
    private readonly DateTimeOffset _timestamp = DateTimeOffset.FromUnixTimeSeconds(1614265330);
    private readonly string _payload = "{\"test\": 2432232314}";
    private readonly string _largePayload = Enumerable.Range(0, 1000).Select(i => i % 10).Aggregate(new StringBuilder(), (sb, i) => sb.Append(i), sb => sb.ToString());
    private readonly Webhook _webhook = new Webhook("whsec_MfKQ9r8GKYqrTwjUPD8ILPZIo2LaLaSw");
    private readonly WebHeaderCollection _headers;
    private readonly WebHeaderCollection _largeHeaders;
    
    public Tests()
    {
        var now = DateTimeOffset.UtcNow;
        _headers = new WebHeaderCollection
        {
            {"svix-id", _msgId},
            {"svix-timestamp", now.ToUnixTimeSeconds().ToString() },
            {"svix-signature", _webhook.Sign(_msgId, now, _payload)}
        };
        _largeHeaders = new WebHeaderCollection
        {
            {"svix-id", _msgId},
            {"svix-timestamp", now.ToUnixTimeSeconds().ToString() },
            {"svix-signature", _webhook.Sign(_msgId, now, _largePayload)}
        };
    }
    
    [Benchmark]
    public string? Sign()
    {
        return _webhook.Sign(_msgId, _timestamp, _payload);
    }
    
    [Benchmark]
    public string? SignLargePayload()
    {
        return _webhook.Sign(_msgId, _timestamp, _largePayload);
    }
    
    [Benchmark]
    public void Verify()
    {
        _webhook.Verify(_payload, _headers);
    }
    
    [Benchmark]
    public void VerifyLargePayload()
    {
        _webhook.Verify(_largePayload, _largeHeaders);
    }
}

Original Code

| Method             | Mean       | Error    | StdDev   | Gen0   | Allocated |
|------------------- |-----------:|---------:|---------:|-------:|----------:|
| Sign               |   945.0 ns |  3.75 ns |  3.13 ns | 0.1469 |     928 B |
| SignLargePayload   | 1,568.2 ns | 15.73 ns | 13.94 ns | 0.6142 |    3864 B |
| Verify             | 1,158.8 ns |  4.16 ns |  3.69 ns | 0.2117 |    1328 B |
| VerifyLargePayload | 1,772.6 ns |  5.76 ns |  4.49 ns | 0.6790 |    4264 B |

Code from this PR

| Method             | Mean       | Error   | StdDev  | Gen0   | Allocated |
|------------------- |-----------:|--------:|--------:|-------:|----------:|
| Sign               |   466.2 ns | 0.68 ns | 0.57 ns | 0.0267 |     168 B |
| SignLargePayload   |   885.4 ns | 3.20 ns | 2.68 ns | 0.0267 |     168 B |
| Verify             |   705.6 ns | 2.24 ns | 1.98 ns | 0.0095 |      64 B |
| VerifyLargePayload | 1,138.2 ns | 3.26 ns | 2.72 ns | 0.0095 |      64 B |

@esskar esskar requested a review from a team as a code owner January 5, 2025 12:17
Copy link
Member

@tasn tasn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for the contribution, and sorry to hear the current code was thrashing the GC.

I made some comments about specific changes (not all of them yet), though would you mind opening smaller PRs that are more focused on just one change so that things are easier to review?

At the moment there are 4+ completely separate changes here which make it very hard to: (1) understand the reasoning behind each change / need, and (2) understanding what's going on so that we can review it effectively.

Thanks a lot!

csharp/Svix/Utils.cs Outdated Show resolved Hide resolved
csharp/Svix/Webhook.cs Outdated Show resolved Hide resolved
csharp/Svix/Webhook.cs Outdated Show resolved Hide resolved
csharp/Svix/Svix.csproj Outdated Show resolved Hide resolved
@esskar
Copy link
Contributor Author

esskar commented Jan 5, 2025

Hey, thanks for the contribution, and sorry to hear the current code was thrashing the GC.

I made some comments about specific changes (not all of them yet), though would you mind opening smaller PRs that are more focused on just one change so that things are easier to review?

At the moment there are 4+ completely separate changes here which make it very hard to: (1) understand the reasoning behind each change / need, and (2) understanding what's going on so that we can review it effectively.

Thanks a lot!

Will do. I will also make some comments to your questions here.

@esskar
Copy link
Contributor Author

esskar commented Jan 6, 2025

I concentrated on the span vs string changes in this PR only; also added benchmarks to see the real benefit

@esskar esskar requested a review from tasn January 6, 2025 09:04
tasn pushed a commit that referenced this pull request Jan 6, 2025
… WebHeaderCollection (#1617)

Provide an overload to the `Webhook.Verify` function to except an
`Func<string, string>` function that acts as an function provider.

<!--
Thank you for your Pull Request. Please provide a description above and
review
the requirements below.

Bug fixes and new features should include tests.
-->

## Motivation

Creating an extra `WebHeaderCollection` everytime a webhook signature
needs to get verified is unnecessary overhead for environments where the
`WebHeaderCollection` is not part of underlying http stack; The object
needs to get created just for the verification.

## Solution

Allowing to pass a header provider function to avoid creating a
WebHeaderCollection in asp.net core environments

See also #1616
@tasn
Copy link
Member

tasn commented Jan 6, 2025

Can you please rebase over main? I merged your other PR, and I'd love to see this PR without those changes for easier review.

csharp/Svix/Utils.cs Outdated Show resolved Hide resolved
@esskar esskar force-pushed the webhook_reduce_memory_and_increase_performance branch from ffa916b to fb5b361 Compare January 6, 2025 15:08
@esskar esskar requested a review from tasn January 6, 2025 15:10
@esskar esskar requested a review from tasn January 9, 2025 11:20
Copy link
Member

@tasn tasn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made one last comment that I found in the latest review, but looks good otherwise.

csharp/Svix/Webhook.cs Outdated Show resolved Hide resolved
@tasn tasn merged commit 18cf625 into svix:main Jan 10, 2025
7 checks passed
@tasn
Copy link
Member

tasn commented Jan 10, 2025

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants